Skip to content

ngclient: Fix symlink usage on Windows - #2981

Merged
jku merged 4 commits into
theupdateframework:developfrom
jku:workaround-symlink-on-windows
Aug 10, 2026
Merged

ngclient: Fix symlink usage on Windows#2981
jku merged 4 commits into
theupdateframework:developfrom
jku:workaround-symlink-on-windows

Conversation

@jku

@jku jku commented Aug 7, 2026

Copy link
Copy Markdown
Member

Running ngclient on Windows (I believe NTFS only, non-privileged account only) leads to
OSError: [WinError 1314] A required privilege is not held by the client

symlinking is apparently a high privilege operation: let's add a fallback, a real file will work just as well.

Fixes #2980

PR is partially AI generated

@jku
jku requested a review from a team as a code owner August 7, 2026 08:03
Running ngclient on Windows (I believe NTFS only) leads to
  OSError: [WinError 1314] A required privilege is not held by the client

symlinking is apparently a high privilege operation: let's add a fallback.

Signed-off-by: Jussi Kukkonen <jkukkonen@google.com>
@jku
jku force-pushed the workaround-symlink-on-windows branch from 79ab351 to 29a7c5b Compare August 7, 2026 08:08

@lukpueh lukpueh left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix!

Comment thread tuf/ngclient/updater.py Outdated
os.symlink(current, linkname)
try:
os.symlink(current, linkname)
except OSError:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this not too broad?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kind of... but I don't actually have a Windows/NTFS machine to test the more complicated code that would check for specific error codes, and the workaround should be harmless even if it happens for other reasons: just an extra copy of root.json

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works for me.

@Phantom9869

Phantom9869 commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Tested on Windows (Python 3.13.12, NTFS) without Developer Mode:

With shutil.copyfile fallback: 196/198 tests pass but test_intermediate_root_cache and test_load_metadata_from_cache fail due to unexpected extra root.json 'wb' writes that the test mocks catch.

Replacing shutil.copyfile with os.link fixes both issues — all 198 tests pass without Developer Mode and no test changes needed.

@jku

jku commented Aug 10, 2026

Copy link
Copy Markdown
Member Author

Tested on Windows (Python 3.13.12, NTFS) without Developer Mode:

With shutil.copyfile fallback: 196/198 tests pass but test_intermediate_root_cache and test_load_metadata_from_cache fail due to unexpected extra root.json 'wb' writes that the test mocks catch.

Replacing shutil.copyfile with os.link fixes both issues — all 198 tests pass without Developer Mode and no test changes needed.

Thanks, this is much appreciated -- figuring this out without the system in question is painful.

* using shutil.copyfile works but tests that mock os.open
  start misbehaving (only on NTFS, so we don't see this on CI)
* shutil.copyfile is also not atomic (something we strive for in write ops)
  but os.link is

Use os.link() in the workaround, it seems better all around.

Signed-off-by: Jussi Kukkonen <jkukkonen@google.com>
@jku

jku commented Aug 10, 2026

Copy link
Copy Markdown
Member Author

Switched NTFS workaround to os.link:

  • using shutil.copyfile works but tests that mock os.open start misbehaving (only on NTFS, so we don't see this on CI)
  • shutil.copyfile is also not atomic (something we strive for in write ops) but os.link is

@jku

jku commented Aug 10, 2026

Copy link
Copy Markdown
Member Author

AI tells me we could actually test this in GH actions. I'll mark this draft while I look into it

@jku
jku marked this pull request as draft August 10, 2026 06:40
jku added 2 commits August 10, 2026 09:49
Signed-off-by: Jussi Kukkonen <jkukkonen@google.com>
Only handle the "required privilege is not held by client"
error, not other OSErrors

Signed-off-by: Jussi Kukkonen <jkukkonen@google.com>
@jku

jku commented Aug 10, 2026

Copy link
Copy Markdown
Member Author

GitHub workflow now runs tests on an unprivileged account on Windows:

  • all of that powershell is from AI
  • I have tested this commit in in a separate branch: the "required privilege is not held by client" error now comes up on CI without the first commit in this PR
  • This means we can now handle the specific error only in _update_root_symlink (because we can now test that code) so I've added that

@jku
jku marked this pull request as ready for review August 10, 2026 07:03
Comment thread tuf/ngclient/updater.py
try:
os.symlink(current, linkname)
except OSError as e:
if getattr(e, "winerror", None) == 1314:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like that better. Thanks!

@jku
jku merged commit 1657e0b into theupdateframework:develop Aug 10, 2026
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

symlinks only work on windows/ntfs with elevated permissions

3 participants